Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[types/EuiButtonEmpty] add missing isLoading and href props #992

Merged
merged 5 commits into from
Jul 11, 2018
Merged

[types/EuiButtonEmpty] add missing isLoading and href props #992

merged 5 commits into from
Jul 11, 2018

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Jul 10, 2018

Tried to use <EuiButtonEmpty /> in typescript but the href prop wasn't typed, pulled over the isLoading prop too because it seemed to be missing but is defined in the propTypes

@spalger spalger requested a review from cchaos July 10, 2018 20:40
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks right to me. Would you mind taking it a bit further and adding those props to the other 2 button types as well?

@chandlerprall Any idea why onClick would not have been added to EuiButtonEmptyProps originally and if we should go ahead and add it now?

@spalger Also, be sure to add a changelog entry in CHANGELOG.md (you should just be able to follow the pattern).

@chandlerprall
Copy link
Contributor

@cchaos onClick is covered by combining with ButtonHTMLAttributes further down in the file

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs bugfix entry to CHANGELOG.md

@spalger
Copy link
Contributor Author

spalger commented Jul 11, 2018

@chandlerprall thanks, updated the changelog, removed the requirement for onClick prop since an href without an onClick is totally valid, and added isLoading prop to get them all in sync.

@spalger
Copy link
Contributor Author

spalger commented Jul 11, 2018

So, the latest types I supplied are somewhat undesirable because it changes the type of the onClick() prop to require a handler for React.MouseEvent<HTMLButtonElement | HTMLAnchorElement>, which really isn't necessary if people are relying solely on onClick() and using this as a standard button. I've tried a couple different ways of having TypeScript detect if you're using the href attribute and base onClick() on that, but I haven't figured out a way to do that which doesn't have other negative side effects... Going to try a couple other things but this will probably impact the default <EuiButton /> use case more than it's worth...

@spalger
Copy link
Contributor Author

spalger commented Jul 11, 2018

Okay, these types work for the following situations:

<EuiButton
  href="string"
  onClick={(e: React.MouseEvent<HTMLAnchorElement>) => undefined}
/>
<EuiButton
  onClick={(e: React.MouseEvent<HTMLButtonElement>) => undefined}
/>
<EuiButton
  href={optionalHref}
  onClick={(e: React.MouseEvent<HTMLButtonElement | HTMLAnchorElement>) => undefined}
/>

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM. Nice solution @spalger !

@spalger spalger merged commit 1082089 into elastic:master Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants